Skip to content
This repository has been archived by the owner on Feb 13, 2019. It is now read-only.

Add TerminalBuffer & TerminalCursor, parse \n \r \b, and more escape codes #85

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

ian-mcdowell
Copy link
Contributor

This change adds another layer between the Parser and TerminalTextView called the TerminalBuffer (referred to as the "buffer"). The buffer contains an NSTextStorage (NSMutableAttributedString subclass) that the TerminalTextView reads from.

The buffer is responsible for adding new text that comes in to the proper position in the text storage. It uses the TerminalCursor (referred to as the "cursor") to keep track of where it needs to insert new text.

Parsing

The buffer is now the Parser's delegate, and receives events from the Parser, some of which are:

  • Newline found (\n)
  • Carriage return found (\r)
  • Attributed text found

Depending on what the Parser sends, the buffer will update the cursor's position and/or modify the text storage.

Cursor

The cursor (different than the UITextView's cursor), keeps track of a position within the text storage. It's only valid for a single text storage. It has a readable x,y coordinate, as well as an offset (current distance from the beginning of the storage).

The cursor contains mutating methods that adjust its state, and is not able to be modified except through those methods. Every time the x, y, or offset change, it makes sure to keep the other values in sync.

Example

In this example, I'll demonstrate what happens when a carriage return is received while parsing.

At the beginning, the storage contains a 2-line string, and the cursor is right after the last character of the first word:

text storage = "iPad: \nhello world"
cursor = { x = 5, y = 1, offset = 12 }

Now, when a carriage return is received, the parserDidReceiveCarriageReturn(_:) method is called by the Parser. After this is handled, here is the state:

text storage = "iPad: \nhello world"
cursor = { x = 0, y = 1, offset = 7 }

Now, if more text comes in from the parser - "bonjour", here is the end state:

text storage = "iPad: \nbonjourorld"
cursor = { x = 7, y = 1, offset = 14 }

}

/// This method is called for each UTF-8 character that is received.
/// It should perform state changes based on that character, then
/// return an attributed string that renders the character
private func handle(_ character: Character) -> (output: NSAttributedString?, didEnd: Bool) {
//
// swiftlint:disable cyclomatic_complexity
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to disable the swiftlint error in this method instead of splitting it up into smaller methods. This method basically models a state machine, and doesn't do much other than change states and call delegate methods. It would, in my opinion, reduce readability a lot if I were to move the inner switches into methods and call those.

@louisdh thoughts?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine.

@@ -175,7 +175,7 @@ enum ANSIFontState: Int {

struct ANSITextState {
var foregroundColor: UIColor = UserDefaultsController.shared.terminalTextColor
var backgroundColor: UIColor = UserDefaultsController.shared.terminalBackgroundColor
var backgroundColor: UIColor = .clear
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, switching background colors while terminal is open will not keep remnants of the background color visible where text was printed.

@ian-mcdowell
Copy link
Contributor Author

Tests were added for both new classes. The TerminalBufferTests is pretty limited at the moment, but the TerminalCursorTests has good code coverage.

@ian-mcdowell
Copy link
Contributor Author

ian-mcdowell commented Feb 11, 2018

One regression I didn't address here:

The OutputSanitizer (replace /private/var/mobile/Containers/.../Documents with ~) is incompatible with the new way that text flows into the buffer, as it happens character by character now, much like other terminals handle text.

Does anyone have feedback on what should be done? Is that replacement necessary? We could sanitize after every character is printed, but that seems too expensive.

@louisdh
Copy link
Owner

louisdh commented Feb 12, 2018

The OutputSanitizer is quite necessary for a nice user experience in my opinion. On top of that, it might be required for App Store approval. I'm speculating here, but Apple may not like an app displaying paths containing "/private/var/mobile/...".

@louisdh
Copy link
Owner

louisdh commented Feb 12, 2018

Great work btw, making OpenTerm behave more like established terminals is a nice improvement. Also good to see some tests.

@ian-mcdowell
Copy link
Contributor Author

ian-mcdowell commented Feb 12, 2018

Thanks! Assuming that the sanitizer won't ever need to replace multiple lines at once, we could avoid displaying each character as it comes in. Whenever we receive \r, \n, etc, or the input ends, run the sanitizer then display the full line .

@ian-mcdowell
Copy link
Contributor Author

This is ready to merge now.

  • Fixed a bug with outputting emoji, where the cursor would get offset when used with NSRange
  • Fixed a history saving bug
  • Re-added the OutputSanitizer

@louisdh
Copy link
Owner

louisdh commented Feb 13, 2018

Two unit tests are failing in the latest commit of this branch:
testBasicText
testTextWithNewLine

@ian-mcdowell
Copy link
Contributor Author

Sorry about that, tests were broken with OutputSanitizer change. They are back up now 🙂

* Added libtext.dylib to OpenTerm

* Fixed share stdin
@louisdh
Copy link
Owner

louisdh commented Mar 2, 2018

Is this good to merge?

@ian-mcdowell
Copy link
Contributor Author

We will need additional validation before it ships, as it may break some commands. Lets keep it in this branch for a while, until I have more confidence in its correctness.

@louisdh
Copy link
Owner

louisdh commented May 18, 2018

Now that OpenTerm 2.0 is done, it would be great to get this PR up to date. Would it be possible to fix all these conflicts?

@ian-mcdowell
Copy link
Contributor Author

I’ll take a look ASAP!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants